internal/controller/proxy_controller_test.go (1)

25-25: ⚡ Quick win

Add tests for the new deny/block policy.

The shared cfg from test.CreateTestConfigs(t) doesn't set Auth.ACLS.Policy, so the entire deny-by-default code path introduced in this PR is never executed by TestProxyController. The existing happy-path/path-allow/ip-bypass/user-allow cases all bypass policyResult either via explicit filters or via the bypass/auth-disabled short circuits.

Consider adding at least:

• A test where cfg.Auth.ACLS.Policy = "block" and a request hits a domain with no matching Apps entry — assert it's denied at the IP allow step.
• A test where cfg.Auth.ACLS.Policy = "block" and a request hits a domain with an explicit IP.Allow (or Users.Allow) — assert it succeeds.
• A test with an invalid policy string (e.g., "deny", "") — assert it logs a warning and behaves as the documented default ("allow"). This will also catch the fallback regression flagged in NewAccessControlsService.

Also applies to: 367-367

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/proxy_controller_test.go` at line 25, The tests for
TestProxyController never exercise the new deny-by-default policy because
test.CreateTestConfigs(t) doesn't set cfg.Auth.ACLS.Policy; add new unit tests
that set cfg.Auth.ACLS.Policy = "block" and exercise the code paths around
policyResult: (1) a request to a domain with no matching Apps entry should be
denied at the IP allow step, (2) a request to a domain with an explicit IP.Allow
or Users.Allow should succeed, and (3) a test with an invalid policy string
(e.g., "deny" or "") should assert a warning is logged and behavior falls back
to documented default ("allow") to catch the NewAccessControlsService fallback
regression. Ensure tests reference and assert on the same controller flow
exercised by TestProxyController so policyResult is evaluated rather than
bypassed.

internal/controller/proxy_controller.go (1)

53-72: ⚡ Quick win

Remove unused auth field and constructor parameter from ProxyController.

After the refactor, all authorization checks in proxyHandler delegate to controller.acls (IP bypass, auth-enabled status, user allowed, groups). The auth field on the struct and corresponding auth parameter in NewProxyController are never referenced. Remove them to keep the dependency surface minimal and make intent clearer. Update the wiring in internal/bootstrap/router_bootstrap.go:47 and test setup accordingly.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/proxy_controller.go` around lines 53 - 72, The
ProxyController struct and constructor still include an unused auth dependency:
remove the auth field from the ProxyController struct and remove the auth
parameter (type *service.AuthService) from NewProxyController signature and its
struct literal, then update all call sites that invoke NewProxyController to
stop passing the auth argument (including bootstrap/router wiring and tests) so
the code compiles with the reduced dependency surface; ensure any imports or
test fixtures related only to auth are cleaned up.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/service/access_controls_service.go (1)

71-82: 💤 Low value

Prefer the explicit local-copy pattern when storing a pointer to the loop value.

appAcls = &config takes the address of the range loop variable. Although the immediate break makes this functionally safe, the project convention is to materialize an explicit local copy to make intent obvious and avoid loop-variable capture confusion.

♻️ Proposed refactor

 	var appAcls *model.App
 	for app, config := range service.config.Apps {
+		appConfig := config
 		if config.Config.Domain == domain {
 			service.log.App.Debug().Str("name", app).Msg("Found matching container by domain")
-			appAcls = &config
+			appAcls = &appConfig
 			break // If we find a match by domain, we can stop searching
 		}
 
 		if strings.SplitN(domain, ".", 2)[0] == app {
 			service.log.App.Debug().Str("name", app).Msg("Found matching container by app name")
-			appAcls = &config
+			appAcls = &appConfig
 			break // If we find a match by app name, we can stop searching
 		}
 	}

Based on learnings: "prefer the explicit local copy pattern (e.g., result := config; return &result) rather than returning &<range-variable> directly … the project prefers the explicit copy for clarity and to avoid reviewer confusion about loop-variable capture semantics."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/access_controls_service.go` around lines 71 - 82, The code
takes the address of the range loop variable (config) when assigning appAcls
(appAcls = &config); change this to the explicit local-copy pattern to avoid
loop-variable capture confusion: create a new local variable (e.g., cfg :=
config) and assign appAcls = &cfg instead for both matches (the domain check and
the app-name check) while keeping the break logic and using the existing
service.config.Apps, appAcls, and config identifiers to locate the spots to
change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/service/access_controls_rules.go (1)

189-192: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid mutating shared ACL/config slice backing arrays during merge.

The current append merge can write into existing backing arrays (ctx.ACLs.IP.*) if capacity allows, causing cross-request leakage/corruption of filter state.

Safe merge without aliasing

-	// Merge the global and app IP filter
-	blockedIps := append(ctx.ACLs.IP.Block, rule.Config.Auth.IP.Block...)
-	allowedIPs := append(ctx.ACLs.IP.Allow, rule.Config.Auth.IP.Allow...)
+	// Merge the global and app IP filter (copy into fresh slices)
+	blockedIps := make([]string, 0, len(ctx.ACLs.IP.Block)+len(rule.Config.Auth.IP.Block))
+	blockedIps = append(blockedIps, ctx.ACLs.IP.Block...)
+	blockedIps = append(blockedIps, rule.Config.Auth.IP.Block...)
+
+	allowedIPs := make([]string, 0, len(ctx.ACLs.IP.Allow)+len(rule.Config.Auth.IP.Allow))
+	allowedIPs = append(allowedIPs, ctx.ACLs.IP.Allow...)
+	allowedIPs = append(allowedIPs, rule.Config.Auth.IP.Allow...)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/access_controls_rules.go` around lines 189 - 192, The merge
of IP lists using append (creating blockedIps and allowedIPs from
ctx.ACLs.IP.Block/Allow and rule.Config.Auth.IP.Block/Allow) can mutate the
backing arrays of the shared ctx.ACLs slices; allocate a fresh slice and copy
both source slices into it instead of appending into an existing backing
array—e.g. create a new slice with capacity equal to
len(ctx.ACLs.IP.Block)+len(rule.Config.Auth.IP.Block) (or start from
append([]string{}, ctx.ACLs.IP.Block...) and then append the rule slice), and do
the same for allowedIPs; update the variables blockedIps and allowedIPs
accordingly so ctx.ACLs.IP.* is never mutated.

internal/utils/security_utils.go (1)

84-93: ⚡ Quick win

Regex filter should not fall through to comma-separated check.

When a regex filter is detected (wrapped in /...) but doesn't match, the code falls through to the comma-separated comparison. This creates an edge case where if input is literally the regex string itself (e.g., input "/^admin$/" against filter "/^admin$/"), it would match unexpectedly.

Additionally, filters like / or // would compile an empty regex that matches everything.

♻️ Proposed fix

 	if strings.HasPrefix(filter, "/") && strings.HasSuffix(filter, "/") {
+		if len(filter) < 3 {
+			return false, fmt.Errorf("invalid regex filter: empty pattern")
+		}
 		re, err := regexp.Compile(filter[1 : len(filter)-1])
 		if err != nil {
 			return false, fmt.Errorf("invalid regex filter: %w", err)
 		}
 
-		if re.MatchString(input) {
-			return true, nil
-		}
+		return re.MatchString(input), nil
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/utils/security_utils.go` around lines 84 - 93, When a filter is
wrapped in slashes (the code branch that checks strings.HasPrefix(filter, "/")
&& strings.HasSuffix(filter, "/")), ensure you treat it exclusively as a regex:
validate that the inner pattern is non-empty (e.g., len(filter) > 2 and
filter[1:len(filter)-1] != "") to avoid compiling "/" or "//" into an empty
regex, compile the pattern and if compilation fails return the error, and if
compilation succeeds but re.MatchString(input) is false return (false, nil)
immediately instead of falling through to the comma-separated logic; update the
existing handling around variables filter, input and the regexp.Compile call to
enforce these checks.

internal/service/access_control_rules_test.go (1)

12-181: ⚡ Quick win

Add nil-context/user-context regression cases for all rules.

Given the rule evaluators are security-critical request-path logic, add table rows for ctx == nil and ctx.UserContext == nil (where applicable) to assert deterministic Effect* outcomes and prevent panic regressions.

Also applies to: 183-309, 311-409, 411-518, 520-636, 638-702

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/access_control_rules_test.go` around lines 12 - 181, Add
regression table rows to TestUserAllowedRule (and the other rule tests
mentioned) to cover nil inputs: create two cases — one with ctx == nil and one
with ctx != nil but ctx.UserContext == nil — and assert the rule's Evaluate
method (UserAllowedRule.Evaluate) returns a deterministic Effect (use
EffectAbstain) for both; repeat the same two-row additions for each
corresponding test function covering the other rule types so Evaluate handles
nil ACLContext and nil UserContext without panicking.

internal/service/access_controls_service.go (1)

37-43: ⚡ Quick win

Use an explicit local copy when returning a pointer from for range.

Returning &config from the range variable works but is easy to misread and has loop-variable semantics pitfalls. Prefer a named local copy before taking the address.

Suggested pattern

-			appAcls = &config
+			result := config
+			appAcls = &result
@@
-			appAcls = &config
+			result := config
+			appAcls = &result

Based on learnings: prefer the explicit local copy pattern instead of returning &<range-variable> directly for clarity and to avoid loop-variable capture confusion.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/access_controls_service.go` around lines 37 - 43, The code
takes the address of the for-range variable `config` when assigning to `appAcls`
(e.g., appAcls = &config) which can be confusing and error-prone due to
loop-variable semantics; fix it by creating an explicit local copy (e.g., copy
:= config) inside the loop before assigning its address to `appAcls`, and do the
same in both match branches (the domain-match branch and the app-name branch
where you call service.log.App.Debug and then set appAcls) so the pointer refers
to a stable, clearly named local variable.

internal/controller/proxy_controller_test.go (1)

367-405: ⚡ Quick win

Add at least one controller-level case for the deny ACL policy.

These tests wire the new policy engine correctly, but they still only exercise the default policy. Since the PR's main behavior is deny-by-default, please cover one unauthenticated request and one explicitly authorized request with the policy set to deny.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/controller/proxy_controller_test.go` around lines 367 - 405, Add
controller-level test cases that exercise the deny-by-default ACL: in the tests
loop that creates the Gin router and calls controller.NewProxyController(log,
runtime, group, aclsService, authService, policyEngine), configure the policy
engine to use the deny default and add two cases—one unauthenticated request
expected to be denied (HTTP 403/401) and one request explicitly authorized via
the ACLs/policy (e.g., using aclsService or injecting an allow rule into
policyEngine) expected to succeed; ensure the new cases are added to the
existing tests slice and use the same setup (router, middlewares, recorder) so
they run with the controller instance created by NewProxyController.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/service/access_controls_rules.go (1)

27-29: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Add complete nil guards before all context dereferences.

Several Evaluate methods can panic on nil ctx/nested fields (e.g., ctx.ACLs and provider-specific contexts). This is still a crash-risk on auth paths and should be short-circuited consistently.

Suggested hardening pattern

 func (rule *UserAllowedRule) Evaluate(ctx *ACLContext) Effect {
-	if ctx.ACLs == nil || ctx.UserContext == nil {
+	if ctx == nil || ctx.ACLs == nil || ctx.UserContext == nil {
 		return EffectAbstain
 	}
+	if ctx.UserContext.Provider == model.ProviderOAuth && ctx.UserContext.OAuth == nil {
+		return EffectAbstain
+	}
@@
 func (rule *OAuthGroupRule) Evaluate(ctx *ACLContext) Effect {
-	if ctx.ACLs == nil || ctx.UserContext == nil {
+	if ctx == nil || ctx.ACLs == nil || ctx.UserContext == nil {
 		return EffectAbstain
 	}
@@
 func (rule *LDAPGroupRule) Evaluate(ctx *ACLContext) Effect {
-	if ctx == nil || ctx.UserContext == nil {
+	if ctx == nil || ctx.ACLs == nil || ctx.UserContext == nil {
 		return EffectAbstain
 	}
@@
 func (rule *AuthEnabledRule) Evaluate(ctx *ACLContext) Effect {
-	if ctx.ACLs == nil {
+	if ctx == nil || ctx.ACLs == nil {
 		return EffectDeny
 	}
@@
 func (rule *IPAllowedRule) Evaluate(ctx *ACLContext) Effect {
-	if ctx.ACLs == nil {
+	if ctx == nil || ctx.ACLs == nil {
 		return EffectAbstain
 	}
@@
 func (rule *IPBypassedRule) Evaluate(ctx *ACLContext) Effect {
-	if ctx.ACLs == nil {
+	if ctx == nil || ctx.ACLs == nil {
 		return EffectDeny
 	}

Also applies to: 82-84, 116-118, 145-147, 184-186, 230-232

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/access_controls_rules.go` around lines 27 - 29, Add complete
nil guards at the top of each rule's Evaluate method in access_controls_rules.go
(e.g., UserAllowedRule.Evaluate and the other Evaluate methods in this file) to
short-circuit on nil ctx or nil nested fields (ctx.ACLs, ctx.UserContext,
provider-specific subcontexts) before any dereference; return EffectAbstain on
those checks to avoid panics and keep behavior consistent across all rule
implementations.

internal/service/docker_service.go (2)

89-94: ⚡ Quick win

Consider using explicit copy pattern when returning pointer to loop variable.

While returning &appLabels is safe in Go 1.22+, the explicit copy pattern improves clarity and consistency with project conventions.

📋 Proposed refactor

 		for _, appLabels := range labels.Apps {
 			if appLabels.Config.Domain == appDomain {
 				docker.log.App.Debug().Str("id", inspect.ID).Str("name", inspect.Name).Msg("Found matching container by domain")
-				return &appLabels, nil
+				result := appLabels
+				return &result, nil
 			}
 		}

Based on learnings: prefer the explicit local copy pattern (e.g., result := config; return &result) when returning a pointer to a loop variable for clarity and to avoid confusion about loop-variable capture semantics.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/docker_service.go` around lines 89 - 94, The loop returns a
pointer to the loop variable appLabels which can be made explicit for clarity:
inside the loop over labels.Apps (the block that logs via docker.log.App.Debug()
using inspect.ID and inspect.Name), create a local copy (e.g., result :=
appLabels) and return &result instead of &appLabels so the returned pointer
clearly references a distinct local value rather than the loop variable.

---

97-102: ⚡ Quick win

Consider using explicit copy pattern when returning pointer to loop variable.

While returning &appLabels is safe in Go 1.22+, the explicit copy pattern improves clarity and consistency with project conventions.

📋 Proposed refactor

 		for appName, appLabels := range labels.Apps {
 			if strings.SplitN(appDomain, ".", 2)[0] == appName {
 				docker.log.App.Debug().Str("id", inspect.ID).Str("name", inspect.Name).Msg("Found matching container by app name")
-				return &appLabels, nil
+				result := appLabels
+				return &result, nil
 			}
 		}

Based on learnings: prefer the explicit local copy pattern (e.g., result := config; return &result) when returning a pointer to a loop variable for clarity and to avoid confusion about loop-variable capture semantics.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/docker_service.go` around lines 97 - 102, The loop returns
the address of the loop variable appLabels (for appName, appLabels := range
labels.Apps), which can be confusing; fix by making an explicit local copy
before returning: inside the loop create a new variable (e.g., result :=
appLabels) and return &result instead of &appLabels so callers receive a stable
pointer; update the return in the matching container block where
docker.log.App.Debug() is called.

internal/service/access_controls_rules_test.go (1)

18-22: ⚡ Quick win

Add explicit ctx: nil test cases for all rule suites.

Only TestLDAPGroupRule currently asserts nil-context behavior. Adding ctx: nil cases to the other rule tables would lock in the nil-guard contract and prevent panic regressions.

Also applies to: 199-203, 447-451, 554-559, 674-678

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/access_controls_rules_test.go` around lines 18 - 22, Add an
explicit "nil context" table entry to each rule test suite (the tests :=
[]struct{...} tables in the Test... functions) similar to the existing nil-case
in TestLDAPGroupRule: add an entry with name "nil ctx", ctx: nil, and expected
set to the rule's intended Effect for a nil ACLContext (match whatever the suite
currently treats a nil/empty context as, e.g., Deny or NoEffect). Ensure you add
this entry to every rule test table in the file so each Test... function asserts
nil-context behavior and prevents panic regressions.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes